Anpl 1704 s3 folders use task queue #1204

michaeljcollinsuk · 2023-09-11T08:50:55Z

📝 Summary

This PR closes #ANPL-1204

This PR updates the task handlers to run actions related to create folders, and grant/revoke access to folders.
Revoking access to S3 buckets (users and apps) will also be handled by the task queue

🔍 What should the reviewer concentrate on?

Changes to the task handlers, which moves logic around granting/revoking access to the task handlers
The send_task logic when creating a datasource, which allows us to wait for a db transaction to complete before sending tasks to create the datasource in AWS

🧑‍💻 How should the reviewer test these changes?

Set feature flag for folders to true in settings.yaml
Make sure you have the celery worker running (using redis recommended)
Create a new folder, click “open with AWS” to check you can access
In the AWS console, create a new folder - this should be permitted as you have write access
Back in Control Panel, update access for the user to readonly. Reopen in AWS, try to create a new folder/add file, check that this fails (may take a few secs for permissions to update)
Back in CP, revoke access for the user. Wait a few secs, then use “open with AWS” to try to access the folder, check this fails
Back in CP, add access to the user again. Click open in AWS and keep the tab open. Then in CP, delete the datasource - wait a few secs, and then in the other tab refresh (may need to refresh couple of times) and check that permission has been revoked as part of the delete.
Repeat these steps with the feature flag disabled to test with S3 buckets.

📚 Documentation status

No changes to the documentation are required
This PR includes all relevant documentation
Documentation will be added in the future because ... (see #ANPL-...)

michaeljcollinsuk · 2023-09-12T14:43:58Z

controlpanel/api/tasks/handlers/s3.py

+        self.complete()
+
+
+class S3BucketRevokeUserAccess(BaseTaskHandler):


@ymao2 wanted to discuss with you the need for checking the user running the task when removing access, but also more broadly.

In the case that a user revokes access to an S3 bucket (or folder) then it is possible that we can use the ID of the user making this action to look them up, lookup the datasource object from the DB, and check that the user is a bucket admin for that object. Based on this, we can then grant/reject the request to revoke access to the user.

In the case that a bucket admin deletes a datasource (bucket or folder), we will also call the revoke access action for every user that has access to the datasource. However, by the time the message is picked out of the queue and run, the DB object will already have been deleted - so there is no way that we can check that the user that triggered the task to be created is a bucket admin.

Due to this, as you can see in the code I have not implemented any logic within the task handler to check that the user who triggered the task is a bucket admin. Doing so is not possible due to the cascading nature of the deletes that occurs when a user deletes a datasource.

However, I think this is actually ok because the only way for a user to trigger the delete (or revoke access) is via the control panel views - and these views already implement permission checks to ensure that the user has correct permissions to revoke/delete a datasource. Which means that by the time that a delete/revoke occurs, we should already have checked the user has permissions. But what do you think, is this a reasonable assumption to make?

This also made me think more generally about the other tasks, and if we really need to pass the user ID when sending a task message (which we currently do by passing the current_user in various places)? Because wherever we are exposing an action that triggers a task to be added to the queue through a user action, permissions should already have been checked. Of course for some tasks we may want to enforce some verification before the action is run, but everything we have implemented so far I am not sure that it is necessary.

Long message so can discuss on slack/call if easier.

I think there are a few things raised from the above case, probably need some discussions

How the Db object life cycle related to the related external resources (tasks)?
if we consider the db as the place for keeping the facts, then it would be ideal all the db records should be in placed before firing the tasking, the records will be removed only when the external resources are removed. --> but I know it may make the coding more complicated.

What sort of validation should be done on tasks? this one is related to your question whether we should validate the permission, I think it is really depending on individual task, for our cases so far, as we validate them against the Control panel which in a way are duplicated as you said, I am happy to not validate it
but just bear in mind, in the future we will separate the tasks handler part out, and also task may be triggered by other components other than cpanel

As discussed on call, I have removed the permission checks in the task handlers, as these are currently repeating checks that have already occurred. Instead, validation/permission checks should be optionally added per-task based on the requirements of running the task itself. There is further refactoring that could be done to simplify the handlers, but will tackle this seperately.

Also, when the soft_delete functionality is added, this will make it easier to add any checks e.g. when revoking access after deleting a datasource.

controlpanel/api/models/s3bucket.py

…reated

…d for transaction lock in the view

…oves need for transaction lock in the view" This reverts commit a8564b8.

…asks when objects committed

…check tasks are sent when creating datasources

Current implementation of the permission checks are unnecessary as they are simply repeating checks that have already occurred in the views that trigger creating tasks. Permissions should instead be implemented as required, where necessary.

ymao2

Thank you for the changes, looks great! 👍 👍
I tested your PR locally, all look good ( but didn't do full set of testings 😊 )

LGTM :)

michaeljcollinsuk commented Sep 12, 2023

View reviewed changes

controlpanel/api/models/s3bucket.py Outdated Show resolved Hide resolved

michaeljcollinsuk force-pushed the ANPL-1704-s3-folders-use-task-queue branch from 70059a7 to 07550e5 Compare September 13, 2023 16:01

michaeljcollinsuk added 19 commits September 13, 2023 17:01

ANPL-1704 fix bug where task to create bucket is sent before bucket c…

aab6dff

…reated

ANPL-1704 use send_task flag on UserS3Bucket

b52237e

ANPL-1704 create the UserS3Bucket in CreateS3Bucket task. Removes nee…

e95539b

…d for transaction lock in the view

ANPL-1704 remove send_task attr from UserS3Bucket

36f6c84

Revert "ANPL-1704 create the UserS3Bucket in CreateS3Bucket task. Rem…

79fa2d6

…oves need for transaction lock in the view" This reverts commit a8564b8.

ANPL-1704 revert to using transaction lock in the view, and sending t…

6809e8c

…asks when objects committed

ANPL-1704 fix test

69d55b4

ANPL-1704 pass bucket owner to CreateS3Bucket task

d106460

ANPL-1704 remove debug prints

94f3871

ANPL-1704 refactor task handlers

eed4c2c

ANPL-1704 update help text for migrating_customers command

b513d87

WIP add comments about github error

a1598ff

ANPL-1704 add task to revoke user access

c6bd7d1

ANPL-1704 add task to revoke bucket access, not used yet

0071a4c

Fix test since changing to tasks to revoke access

432e6cf

ANPL-1704 enable revoke app access via a task

6870167

ANPL-1704 use handle method on all tasks

c43582d

ANPL-1704 add tests for new tasks to revoke access

6a310a0

NPL-1704 remove send_task flag to simplify the flow. Update tests to …

928238f

…check tasks are sent when creating datasources

michaeljcollinsuk force-pushed the ANPL-1704-s3-folders-use-task-queue branch from 07550e5 to 928238f Compare September 13, 2023 16:02

ANPL-1704 Pass the current_user when revoking access to a datasource

901a2b8

michaeljcollinsuk force-pushed the ANPL-1704-s3-folders-use-task-queue branch from 22a503e to 901a2b8 Compare September 13, 2023 16:35

michaeljcollinsuk marked this pull request as ready for review September 14, 2023 11:35

michaeljcollinsuk requested a review from ymao2 September 14, 2023 11:39

ymao2 approved these changes Sep 15, 2023

View reviewed changes

michaeljcollinsuk merged commit 1bb86dd into main Sep 15, 2023

michaeljcollinsuk deleted the ANPL-1704-s3-folders-use-task-queue branch September 15, 2023 15:00

michaeljcollinsuk mentioned this pull request Sep 18, 2023

ANPL-1704 enable folders feature in dev for testing #1205

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Anpl 1704 s3 folders use task queue #1204

Anpl 1704 s3 folders use task queue #1204

michaeljcollinsuk commented Sep 11, 2023 •

edited

Loading

michaeljcollinsuk Sep 12, 2023 •

edited

Loading

ymao2 Sep 13, 2023

michaeljcollinsuk Sep 14, 2023

michaeljcollinsuk Sep 14, 2023

ymao2 left a comment

		self.complete()


		class S3BucketRevokeUserAccess(BaseTaskHandler):

Anpl 1704 s3 folders use task queue #1204

Anpl 1704 s3 folders use task queue #1204

Conversation

michaeljcollinsuk commented Sep 11, 2023 • edited Loading

📝 Summary

🔍 What should the reviewer concentrate on?

🧑‍💻 How should the reviewer test these changes?

📚 Documentation status

michaeljcollinsuk Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

ymao2 Sep 13, 2023

Choose a reason for hiding this comment

michaeljcollinsuk Sep 14, 2023

Choose a reason for hiding this comment

michaeljcollinsuk Sep 14, 2023

Choose a reason for hiding this comment

ymao2 left a comment

Choose a reason for hiding this comment

michaeljcollinsuk commented Sep 11, 2023 •

edited

Loading

michaeljcollinsuk Sep 12, 2023 •

edited

Loading